Skip to content

Conversation

cadenmyers13
Copy link
Contributor

builds a dictionary with the following structure

{
"pack1": [("ex1", Path(path/to/ex1)), ("ex2", Path(path/to/ex2))],
"pack2": [("ex3", Path(path/to/ex3)), ("ex4", Path(path/to/ex4))],
}

Note: this will fail until #54 is merged because there is no temp dir on this PR

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is very clean and beautiful now, we finally arrived! Nice. Please address the last two comments and I can merge. Let's work the other tests to look as clean and pretty.

]


def paths_and_names_match(expected, actual, root):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls move this helper function to the top of the module



def paths_and_names_match(expected, actual, root):
"""Compare two (example_name, path) lists ignoring temp dir
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are tuples or doubles but not lists

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now I made a review of the actual code, not the test....

with get_package_dir() as pkgdir:
with get_package_dir(root_path) as pkgdir:
pkg = Path(pkgdir).resolve()
for c in (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we know what this is doing? it seems to worry that the pkgdir coming from the init is sometimes something and sometimes something else. Why would that be? Its possible this is needed depending on whether the package is installed using -e or not. @Tieqiong do you comments on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbillinge Not super familiar so I gave chatgpt your comments and it said this. It looks like it does have to do with installing with -e.

Screenshot 2025-10-10 at 11 04 21 AM

It also recommended something more readable like this (I left docstring but we can tune it as desired):

def _installed_packs_dir(root_path: Path | None = None) -> Path:
    """Return the path to the `requirements/packs` directory for this package.

    This function handles both normal and editable (`pip install -e .`)
    installation modes, which place package files in slightly different
    locations on disk.

    Parameters
    ----------
    root_path : Path or None, optional
        Used mainly for testing. Overrides the package directory
        returned by :func:`get_package_dir`.

    Returns
    -------
    Path
        The resolved path to `requirements/packs`.

    Raises
    ------
    FileNotFoundError
        If the `requirements/packs` directory cannot be found.
    """
    with get_package_dir(root_path) as pkgdir:
        pkgdir = Path(pkgdir).resolve()
        installed_layout = pkgdir / "requirements" / "packs"
        editable_layout = pkgdir.parents[1] / "requirements" / "packs"
        for candidate in (installed_layout, editable_layout):
            if candidate.is_dir():
                return candidate

    raise FileNotFoundError(
        f"Could not locate requirements/packs under {pkgdir}. "
        "Check your installation or installation mode (-e)."
    )

Copy link

codecov bot commented Oct 10, 2025

Codecov Report

❌ Patch coverage is 89.28571% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.93%. Comparing base (53318eb) to head (9f4cff9).
⚠️ Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
tests/test_packsmanager.py 89.28% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
+ Coverage   82.16%   82.93%   +0.77%     
==========================================
  Files          12       13       +1     
  Lines        1239     1313      +74     
==========================================
+ Hits         1018     1089      +71     
- Misses        221      224       +3     
Files with missing lines Coverage Δ
tests/test_packsmanager.py 89.28% <89.28%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cadenmyers13
Copy link
Contributor Author

@sbillinge ready for review, all tests passed

@sbillinge sbillinge merged commit 7659ba1 into diffpy:main Oct 12, 2025
5 checks passed
@cadenmyers13 cadenmyers13 deleted the dict-build-test branch October 13, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants